Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: test-utils #644

Merged
merged 8 commits into from
Dec 24, 2019
Merged

feat: test-utils #644

merged 8 commits into from
Dec 24, 2019

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented Dec 22, 2019 •
edited
Loading

Which problem is this PR solving?

#617

Short description of the changes

A common @opentelemetry/test-utils to start and stop docker containers required for tests. The functions require a db argument to indicate which container to start/stop. The allowed values are redis, mysql and postgres.

Currently used in redis, mysql and postgres plugins.
If merged, it would have to be applied to ioredis(not yet merged) as well.

P.S. Excuse the amends/force-pushes I presume nobody has looked at this yet and wanted a cleaner history.

@codecov-io
Copy link

codecov-io commented Dec 22, 2019 •
edited
Loading

Codecov Report

Merging #644 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #644      +/-   ##
==========================================
- Coverage   89.52%   89.52%   -0.01%     
==========================================
  Files         188      188              
  Lines        9656     9691      +35     
  Branches      902      897       -5     
==========================================
+ Hits         8645     8676      +31     
- Misses       1011     1015       +4
Impacted Files Coverage Δ
...ages/opentelemetry-plugin-http/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...ges/opentelemetry-plugin-https/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...es/opentelemetry-node/src/instrumentation/utils.ts 90.47% <0%> (-9.53%) ⬇️
packages/opentelemetry-plugin-mysql/src/utils.ts 90.9% <0%> (-4.55%) ⬇️
...opentelemetry-base/test/resources/resource.test.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/config.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/utility.ts 100% <0%> (ø) ⬆️
...ges/opentelemetry-exporter-zipkin/src/transform.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/test/Span.test.ts 100% <0%> (ø) ⬆️
.../opentelemetry-plugin-dns/test/utils/assertSpan.ts 100% <0%> (ø) ⬆️
... and 13 more

dyladan
dyladan reviewed Dec 23, 2019
View reviewed changes
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add fix and check npm scripts

@naseemkullah
Copy link
Member Author

Please add fix and check npm scripts

✅

@naseemkullah
Copy link
Member Author

Would it make sense to include assertSpan and assertPropagation in the test-utils package?
The ones used in pg and redis are very similar and could potentially be used for future plugins.
e.g.:

https://github.com/open-telemetry/opentelemetry-js/blob/fa583fde07425f0aca75d3902d13e5a7d9a5dc65/packages/opentelemetry-plugin-redis/test/assertionUtils.ts

https://github.com/open-telemetry/opentelemetry-js/blob/2bfa6c028331ca5da5113c5b10c41d045f624939/packages/opentelemetry-plugin-postgres/opentelemetry-plugin-pg/test/assertionUtils.ts

@naseemkullah
Copy link
Member Author

Other plugins such as dns and http have a different assertSpan function however.
e.g. http and dns

opentelemetry-js/packages/opentelemetry-plugin-dns/test/utils/assertSpan.ts

Line 25 in 502f9e4

export const assertSpan = (

https://github.com/open-telemetry/opentelemetry-js/blob/06d21e348476e2be04480ba76e719efb1d242c4b/packages/opentelemetry-plugin-http/test/utils/assertSpan.ts

But the function could potentially work for those as well provided it is configurable to the needs of those plugins.

@dyladan
Copy link
Member

dyladan commented Dec 23, 2019

Would it make sense to include assertSpan and assertPropagation in the test-utils package?
The ones used in pg and redis are very similar and could potentially be used for future plugins.

Yes definitely.

@mayurkale22 mayurkale22 added cleanup Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) labels Dec 23, 2019
@mayurkale22 mayurkale22 added this to the Alpha v0.3.2 milestone Dec 23, 2019
@mayurkale22
Copy link
Member

Could you please do the same thing on pg-pool plugin?

@mayurkale22
Copy link
Member

Would it make sense to include assertSpan and assertPropagation in the test-utils package?
The ones used in pg and redis are very similar and could potentially be used for future plugins.

What's your plan on this? Are you planning to do it separate PR?

@naseemkullah
Copy link
Member Author

naseemkullah commented Dec 23, 2019 •
edited
Loading

Would it make sense to include assertSpan and assertPropagation in the test-utils package?
The ones used in pg and redis are very similar and could potentially be used for future plugins.

What's your plan on this? Are you planning to do it separate PR?

I will include for pg, redis and mysql momentarily. Not sure about other ones such as http and dns, what do you suggest?

@mayurkale22
Copy link
Member

I will include for pg, redis and mysql momentarily. Not sure about other ones such as http and dns, what do you suggest?

I was thinking to handle http and dns use-case in separate PR and not pollute this one. Looks like we need to do some extra refactoring to achieve that. @dyladan WDYT?

OlivierAlbertini
OlivierAlbertini approved these changes Dec 23, 2019
View reviewed changes
Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you!

@naseemkullah
Copy link
Member Author

Alright I think I will stop here with this PR, we can look into adding it to mysql,dns,http and other plugins in a separate PR.

@mayurkale22
Copy link
Member

This is nice, thank you @naseemkullah :)

@mayurkale22 mayurkale22 merged commit 67d2256 into open-telemetry:master Dec 24, 2019
@naseemkullah
Copy link
Member Author

My pleasure @mayurkale22 !

@naseemkullah naseemkullah deleted the test-utils branch December 24, 2019 02:07
@mayurkale22 mayurkale22 mentioned this pull request Jan 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dyladan dyladan dyladan approved these changes

@mayurkale22 mayurkale22 mayurkale22 approved these changes

@markwolff markwolff markwolff approved these changes

@OlivierAlbertini OlivierAlbertini OlivierAlbertini approved these changes

@bg451 bg451 Awaiting requested review from bg451

@obecny obecny Awaiting requested review from obecny

@rochdev rochdev Awaiting requested review from rochdev

@vmarchaud vmarchaud Awaiting requested review from vmarchaud

Assignees
No one assigned
Labels
Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Milestone
Alpha v0.3.2
Development

Successfully merging this pull request may close these issues.
6 participants
@naseemkullah @codecov-io @dyladan @mayurkale22 @OlivierAlbertini @markwolff